fix(watcher): re-evaluate arm coalescing when the fleet lock changes - #14
Merged
Merged
Conversation
Two independent causes made the arm-readiness suite fail a different assertion almost every run: - ensureArm() in the OpenCode watch plugin reused a still-resolving earlier caller's beginArm() result unconditionally. Every ordinary session.idle produces two callers, so when the fleet lock was reacquired while an earlier attempt was mid-flight, the later caller inherited that attempt's stale read-only verdict and never armed. Fixed with premise-validated coalescing: a caller shares an in-flight attempt only while the lock file content it captured is still current; otherwise it evaluates fresh. Two callers on an unchanged lock still coalesce into one subprocess walk, so this costs nothing on the ordinary turn a serialized-everything fix would have doubled. - Both adapters spawn their arm child through a login shell, which sources /etc/profile in addition to the account's own profile files; the system-wide half is not relocatable via HOME. main already raised the readiness timeout to absorb that cost (250ms to 2000ms) and its own measurement shows a worst case of ~1740ms under contention against that budget - narrower headroom, not a removed confound. This adds FM_WATCH_ARM_NO_LOGIN_SHELL, a test-only opt-out that spawns the arm child under plain bash -c, removing the confound instead of padding around it. Production keeps the login shell as the unconditional default. Also fixes three unhandled-EPIPE crash sites found while proving this change under load: child.stdin.end() in fm-primary-turnend-guard.js, fm-primary-turnend-guard.ts, and fm-operational-input.js raises an unhandled 'error' event on the stdin stream (not the ChildProcess) when the child exits before the write lands, which was crashing the whole session process. Each site now no-ops that stream error since the child's own close/error handlers already drive resolution. Adds two regression tests (opencode coalescing on an unchanged lock, login-shell default vs. opt-out for both adapters) and one EPIPE regression test, and rewrites the existing OpenCode lock test to force the stale-verdict race deterministically via a gated ps shim instead of waiting for it. See docs/arm-readiness-determinism-proof.md for the repeated-run proof and docs/configuration.md for the new FM_WATCH_ARM_NO_LOGIN_SHELL entry. Built directly on current main (45bd292); does not touch main's own prior timeout raise or its test_pi_session_transition_generation_owner fixture-ordering fix, both kept as-is.
…tore expect_code diagnostics
…ll residual bound
…with provenance
joliverMI
added a commit
that referenced
this pull request
Aug 21, 2026
* docs(arm-readiness): record live post-merge verification that main is green A report claimed main's CI had been red on the portable serial shards for at least 11 hours with merges landing on top of it. Live CI evidence, the prior red-streak's own CI logs, and a fresh local rerun of the suite all confirm this describes the incident PR #14 already fixed, not main's current state: HEAD's CI run is fully green across all shards, nothing has touched the arm/watch code since the fix merged, and the suite reproduces clean on 10/10 idle and 9/10 loaded local runs (the one loaded failure is an assertion outside the fix's scope, consistent with this doc's own noted host-dependent load sensitivity). * no-mistakes(review): Correct overstated attribution claims in arm-readiness addendum * no-mistakes(review): Complete card-link test touch list in arm-readiness addendum * no-mistakes(review): Correct cancelled-run explanation and unattributed-failure conclusion * no-mistakes(review): Cite all eight streak jobs and fix intro wording * no-mistakes(review): Correct residual misfiling, load comparison, and red-window framing * no-mistakes(review): Stop asserting unevidenced mechanism for FM_HOME load failure * docs(verification): reframe the finding around the CI-red merge blind spot Firstmate redirected the deliverable: main is green (a prior fix already landed and holds), so re-verifying it isn't the durable value here - the detection gap is. Revert the arm-readiness-suite addendum (out of that doc's own stated scope, and centered on the wrong thing) and add a properly-scoped record instead: main has no branch protection, so no CI result - green or red - has ever been able to stop a merge here; the red period was two ~22-24h windows totaling ~45h, not the reported 11h; and gh-axi/gh silently default to the upstream parent repo without an explicit --repo flag, which is itself part of why nobody caught this. * no-mistakes(review): Correct eight factual claims in CI-red blind-spot record * no-mistakes(review): Correct pre-merge CI mechanism and window B attribution * no-mistakes(review): Fix window B count, date snapshot, name unconfirmed failure * no-mistakes(review): Drop disproved mechanism, scope the no-notification claim * no-mistakes(review): Name timeout shard, drop branch SHA, cite real pre-merge green * no-mistakes(review): Correct shard attribution for both CI job timeouts * no-mistakes(review): Complete the shard map; name incomplete report premise * no-mistakes(document): Trim task chronology from CI-red blind-spot record --------- Co-authored-by: joliverMI <joliver@sensibletech.biz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Make the arm-readiness suite (tests/fm-pi-watch-extension.test.sh) deterministic. It had been red on main for over a day, failing a different assertion almost every run: 'Pi must fall back without overlapping an unretired successor', 'Pi must deliver the actionable wake after bounded hung-successor recovery', and 'OpenCode watch plugin must arm only when this session owns the fleet lock' (twice). Establish why it is timing-dependent before changing anything: is the test polling for a condition it cannot observe, or is production code genuinely racy - these need opposite fixes. Do not just raise the readiness/retire timeout values again as a fix for a genuine race (a prior attempt already tried that and it did not work; note that this branch's base, main, independently already raised those specific values as its own separate change before this branch existed - this change does not revert or re-litigate that raise, and does not itself raise any timeout value). If the test is at fault, make it wait on a real observable condition rather than elapsed time. If production code is racy, fix the race and say so loudly rather than papering over it with a wider window. Prove the fix by running the suite repeatedly (at least 20 consecutive runs on an idle machine and at least 20 consecutive runs under substantial simulated CPU load) and report the exact pass/fail counts as counts, not as a rounded-up verdict. Report which of the four originally-failing assertions share one cause and which are separate. Do not disable, skip, or mark any test flaky to make CI green. The suite runs on a shared machine under variable load, so a fix that only works on an idle machine is not a fix. If any regression test or proof claim's own supporting evidence stops matching the implementation it describes as the implementation evolves, correct the claim rather than leaving it stale - this has been a recurring failure mode tonight and gets no exception here.
What Changed
ensureArmin.opencode/plugins/fm-primary-watch-arm.jsno longer reuses an in-flightbeginArm()result unconditionally: it snapshotsstate/.lockat call time and shares the in-flight attempt only while that content still matches what the attempt captured, so a caller arriving after the lock was reacquired no longer inherits a staleread-onlyverdict and skips arming. Callers on an unchanged lock still coalesce into a singlegit/pswalk.FM_WATCH_ARM_NO_LOGIN_SHELL=1opt-out (both the OpenCode plugin and.pi/extensions/fm-primary-pi-watch.ts) that spawns the arm child underbash -cinstead ofbash -lc, keeping the unbounded/etc/profilesourcing cost out of timed readiness windows; the login shell stays the production default, and the new variable is documented indocs/configuration.md. Also addedchild.stdin.on("error")guards at the three asyncstdin.endsites in the adapters, where an early-exiting child raised an unhandled EPIPE that killed the host session process.tests/fm-pi-watch-extension.test.sh: the four fallback cases now wait on observedarm=<pid>row counts instead of elapsed time and re-read the log afterward to catch stray extra arms; the lock-ownership case was rewritten to force the race deterministically via a gatedpsshim; and new cases pin unchanged-lock coalescing and both login-shell branches.tests/fm-operational-input.test.shgained an EPIPE host-survival case, anddocs/arm-readiness-determinism-proof.mdrecords the cause attribution plus the repeated-run counts (20/20 idle, 20/20 under 5x load).Risk Assessment
Testing
Ran the arm-readiness suite 20 times idle and 20 times under 5x CPU oversubscription against 9c67804 - 40/40 clean with zero failing assertions - while the pre-change copy of the same suite failed all 4 runs under identical load, so the determinism claim reproduces end-to-end on this host. Independently re-measured the login-shell contention figures the corrected proof now cites (idle median 130ms, loaded median 1740ms / max 4527ms) and confirmed every headroom conclusion the change draws from them still holds on fresh numbers, including that the loaded worst case exceeds main's 2000ms budget and that the residual 10s bound is ~2.2x rather than "well above". Re-reproduced the cause-A and EPIPE regression claims by reverting each fix, observing the exact failure text the proof states, and restoring to green; the rejected-serialized-variant row was verified in round 1 and is unchanged here. Also ran the operational-input suite, which owns the EPIPE regression case. No visual artifact is possible - this is a shell/node test-harness and adapter change with no rendered UI surface - so the reviewer-visible evidence is the run-count transcript and measurement tables.
Evidence: Determinism run counts: 20/20 idle, 20/20 loaded, 0/4 pre-change control
Source: Determinism run counts: 20/20 idle, 20/20 loaded, 0/4 pre-change control
== Phase 1: IDLE (1-min load average 0.9 at phase start) == TAG=idle PASS=20 FAIL=0 TOTAL=20 == Phase 2: LOADED (160 busy loops on 32 cores = 5x oversubscription; load average 141 -> 162) == TAG=loaded PASS=20 FAIL=0 TOTAL=20 == Control: PRE-CHANGE suite copy (45bd292), same host, same 5x load == run 1: FAIL not ok - Pi redundant tool call must remain an ownership-based no-op with repair-only guidance: expected exit 0, got 1 run 2: FAIL not ok - Pi redundant tool call must remain an ownership-based no-op with repair-only guidance: expected exit 0, got 1 run 3: FAIL not ok - Pi redundant tool call must remain an ownership-based no-op with repair-only guidance: expected exit 0, got 1 run 4: FAIL not ok - Pi redundant tool call must remain an ownership-based no-op with repair-only guidance: expected exit 0, got 1 TAG=baseline PASS=0 FAIL=4 TOTAL=4Evidence: Independent re-measurement of the corrected login-shell contention figures
Source: Independent re-measurement of the corrected login-shell contention figures
IDLE (load average ~0.4): bash -lc true n=20 min=128ms median=130ms max=139ms bash -c true n=20 min=1ms median=1ms max=1ms LOADED - 160 busy loops on 32 cores (5x), load average ~155: bash -lc true n=30 min=1289ms median=1740ms max=4527ms bash -c true n=20 min=1ms median=5ms max=20ms Cited by the change: idle ~131ms median; loaded min 1096 / median ~1620 / max 4246ms -> reproduces within load-to-load variance. Conclusions checked: loaded worst case > main's 2000ms budget (4527 > 2000) HOLDS; 5s budget leaves ~10% headroom HOLDS; 10s residual bound is ~2.2x the worst case HOLDS.Evidence: Regression claims re-reproduced by reverting each fix
Source: Regression claims re-reproduced by reverting each fix
== Revert ensureArm to pre-change unconditional in-flight reuse == Error: the reacquired-lock arm attempt never settled not ok - OpenCode watch plugin must arm only when this session owns the fleet lock: expected exit 0, got 1 (the coalescing case run alone against the SAME reverted code still passes - it does not itself catch cause A) == Remove child.stdin.on("error") EPIPE guard == not ok - OpenCode adapter host died (exit 1) when the encoder exited before reading the body: Error: write EPIPE Both returned to green after restoring the fixes.Evidence: Round-2 evidence summary (markdown)
Source: Round-2 evidence summary (markdown)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed (7) ✅
tests/fm-pi-watch-extension.test.sh:503- Intent conformance: the criteria state "If the test is at fault, make it wait on a real observable condition rather than elapsed time." The change classifies the two Pi assertions as cause B ("the test measured something it did not intend to" - i.e. test at fault), but the remedy is to shrink the cost inside the window (FM_WATCH_ARM_NO_LOGIN_SHELL=1 at line 51) rather than to make the test wait on an observable condition. Both cases remain elapsed-time dependent: test_pi_hung_successor_falls_back_to_typed_wake asserts rows.length !== 4 (line 503), which requires each of the three unready arm children to append itsarm=<pid>row before the extension SIGTERMs it at FM_PI_ARM_READY_TIMEOUT_MS=2000; test_pi_unretired_successor_falls_back_without_retry asserts rows.length !== 2 (line 575) with the same dependence. Removing /etc/profile sourcing narrows the constant but does not remove the dependence - a sufficiently descheduled bash -c + exec still loses the row. The author documents this deliberately in docs/arm-readiness-determinism-proof.md and backs it with 20/20 under 5x oversubscription, so this is a judgment call for the user, not something to resolve in review.docs/arm-readiness-determinism-proof.md:38- The proof document contradicts itself on which cause the login-shell opt-out addresses. Its own table (lines 11-13) assigns the login shell to cause B ("test window racing an unrelated cost") and reserves cause A for the ensureArm race; the "What this change adds" bullet at line 38 then describes FM_WATCH_ARM_NO_LOGIN_SHELL as "a further reduction of cause A's residual exposure alongside main's own timeout raise". main's timeout raise and the profile-sourcing cost are both cause B; cause A is fixed outright by the coalescing change described in the bullet immediately above. Since the intent makes stale/incorrect proof claims a no-exception item, this should read "cause B".tests/fm-pi-watch-extension.test.sh:25- The pre-existing header comment block (lines 25-30) is now stale supporting evidence. It cites the implementation asspawn("bash", ["-lc", ...])in both adapters - the source now readsspawn("bash", [ARM_SHELL_FLAG, ...])/spawn("bash", [armShellFlag, ...])- and asserts "so every arm pays for /etc/profile and /etc/profile.d before the fixture's fm-watch-arm.sh runs its first line", which is false for this suite now that line 51 exports FM_WATCH_ARM_NO_LOGIN_SHELL=1 for every case except test_watch_arm_login_shell_default_reaches_the_arm_child. The new paragraph at lines 40-50 partially corrects it, but the older block still states the old behaviour as current fact. The intent explicitly requires correcting a claim whose supporting evidence stopped matching the implementation.tests/fm-pi-watch-extension.test.sh:1483- The rewritten lock test dropped the"$out"argument it previously passed to expect_code, so the rich diagnostics the rewrite added are discarded on failure. expect_code (tests/lib.sh:287-293) prints its 4th argument only when non-empty and then calls fail, which exits 1 - so the following[ -z "$out" ] || fail ... "$out"line is never reached on a non-zero status. Concretely: when the node body throwsthe reacquired-lock caller inherited the in-flight foreign-lock verdictortimed out waiting for the foreign-lock ownership walk to block mid-flight, the operator sees only "expected exit 0, got 1". For a suite whose entire purpose here is diagnosing intermittent failures on a shared machine, that is the wrong default. Same omission in the two new tests at lines 1593, 1659, and 1695. Restore"$out"as the 4th expect_code argument at those four call sites.docs/arm-readiness-determinism-proof.md:53- The counterfactual claim for the rewritten lock test is inaccurate about mechanism. It says the test "Fails against the pre-change ensureArm (inherits the stale verdict) and against a rejected fully-serialized variant (deadlocks)". Against the pre-change ensureArm the reacquired-lock caller takes the unconditionallaunchResult = await launchInFlightbranch and blocks on the foreign attempt, which is itself pinned inside the gated ps shim until FM_PS_RELEASE is written - and the test does not write that file until afterawait settling(owned). So pre-change code also fails by the 20ssettlingguard rejecting with "the reacquired-lock arm attempt never settled", not by observing an inherited read-only verdict. The test does discriminate correctly; only the stated reason is wrong, and the intent flags stale proof claims as a no-exception item.🔧 Fix: correct stale arm-readiness proof claims and restore expect_code diagnostics
3 issues (2 warnings, 1 info) still open:
tests/fm-pi-watch-extension.test.sh:1654- The new/changed cases gate on the arm log's EXISTENCE and then assert its CONTENT, leaving a real (if narrow) window on the loaded machine this suite targets.printf 'marker=%s\n' ... >> "$FM_ARM_LOG"opens/creates the file and writes in two steps; a fixture descheduled between them leaves a zero-byte file that the poller accepts. In test_watch_arm_login_shell_default_reaches_the_arm_child the gap is widest because it spans processes: node polls onlyexistsSync(lines 1654 and 1690), exits, and only then does bash rungrep -qx "$expected" "$log"(lines 1666 and 1704) - a failed grep reportsexpected marker=sourced, got:with an empty file. The same shape is at lines 1471-1472 of the rewritten lock test, where it would throw the misleading "the reacquired-lock caller reported success without running the arm". Under the 5x oversubscription the proof document runs under, a preemption between open() and write() is exactly the class of event this change exists to remove. Fix: poll on content, not existence - e.g.waitFor(() => existsSync(log) && readFileSync(log, "utf8").includes("arm"))at 1471, and!readFileSync(log,"utf8").includes("marker=")as the loop condition at 1654/1690.docs/arm-readiness-determinism-proof.md:62- The Verification block's provenance claim no longer names the tree that was measured. Line 62 says "Code under proof: this branch's commit, built directly onmainat45bd292", and line 5 says the numbers "are from a full run against the tree with this change's fixes applied ... and supersede any earlier in-branch run figures produced against a different tree". The 40 runs were performed against d78a403; HEAD is now 2d171b1, which edited tests/fm-pi-watch-extension.test.sh after the fact. Those edits are behaviour-neutral for the counts (a 4th"$out"argument to expect_code, which expect_code only reads on failure, plus comment rewording), so the 40/40 figures still hold - but the document's own stated standard is that a proof claim whose supporting evidence stops matching the tree it describes gets corrected rather than left stale, and the intent marks that a no-exception item. Either name the measured commit explicitly and note that the later edits were diagnostic-only, or re-run the two phases against HEAD.tests/fm-pi-watch-extension.test.sh:1523- test_opencode_primary_watch_plugin_requires_session_lock (lines 1398-1413) and test_opencode_watch_arm_coalesces_callers_on_an_unchanged_lock (lines 1523-1536) install byte-identicalpsgate andgitcounter shims, and their node bodies repeat identicalwaitFor,settling, andevaluationshelpers. Theevaluations()helper in particular encodes a brittle invariant ("isPrimaryRoot runs exactly two rev-parse probes", so divide the log line count by 2) in two places, so a future change to isPrimaryRoot's probe count silently breaks one copy's arithmetic in a way the other copy's comment still documents. Extracting a singleinstall_arm_gate_shims <prefix>bash helper that echoes the fakebin and exports the gate/entered/release/gitlog paths would keep that invariant in one place. Non-blocking; the suite's house style is per-test inline fixtures.🔧 Fix: wait on observable arm rows and share gate shims
4 issues (2 warnings, 2 infos) still open:
docs/arm-readiness-determinism-proof.md:68- The Verification block's 40 runs no longer cover the tests they claim to prove. Line 5 says the numbers "are from a full run against the tree with this change's fixes applied" and line 68 says "Code under proof: this branch's commit, built directly onmainat45bd292", but the 20-idle/20-loaded phases were run at d78a403. Commit 5f56cc7 then rewrote the wait structure of all four fallback cases (tests/fm-pi-watch-extension.test.sh:512-527, 609-624, 2038-2053, 2137-2152), replacing the single elapsed-time loop with a row-count wait plus a separate bounded prompt wait - and the same commit's own new bullet (lines 44-49) calls that "the other half of the cause B fix, and the one that removes the elapsed-time dependence rather than shrinking it". So the change's headline determinism mechanism has zero repeated-run evidence behind it, while the doc presents 40/40 as if it did. The intent requires "Prove the fix by running the suite repeatedly (at least 20 consecutive runs on an idle machine and at least 20 consecutive runs under substantial simulated CPU load) and report the exact pass/fail counts" and states "If any regression test or proof claim's own supporting evidence stops matching the implementation it describes as the implementation evolves, correct the claim rather than leaving it stale - this has been a recurring failure mode tonight and gets no exception here." The round-2 instruction also explicitly ordered this as a final pass: rerun both phases against the final tree and rewrite the Verification section with that head SHA and fresh counts. That was not done - 5f56cc7 touched only the "What this change adds" section. Note the restructure looks like a strict headroom improvement (each fallback case now has two sequential budgets instead of one covering the whole sequence), so this is an evidence gap rather than a suspected regression; it needs the rerun the criteria require, plus the concrete measured SHA named on line 68. Assertions per run (32) is still accurate.tests/fm-pi-watch-extension.test.sh:540- The four assertions this whole change exists to stabilize discard their failure output. tests/lib.sh:281-293 documents an optional 4th <output> argument to expect_code that prints captured stdout/stderr before calling fail (which exits 1, so the following[ -z "$out" ] || fail ... "$out"line is unreachable on a non-zero status). The fix round rewrote these four node bodies and added their most informative messages -expected one successor plus two retries, got 3: arm=11 | arm=12 | arm=13andunretired arm overlapped a retry: ...- but the expect_code calls at lines 540, 633, 2066 and 2161 still pass only three arguments, so an operator seeing one of these flake on the shared machine gets only "expected exit 0, got 1". Round 1 restored this argument for the rewritten lock/coalescing/login-shell cases (lines 1550, 1644, 1712, 1750); these four were rewritten in the same change and are the exact assertions named in the intent as having been red for over a day, so they are in scope rather than an unrelated repo-wide cleanup. Add "$out" as the 4th argument at those four call sites.tests/fm-pi-watch-extension.test.sh:615- In test_pi_unretired_successor_falls_back_without_retry (and its OpenCode counterpart at line 2143) the restructured wait breaks onrows.length >= 2and then assertsrows.length !== 2, so at the instant it fires the assertion can only be satisfied - it is a lower bound checked against the condition that ended the loop. Previously the count was taken after the prompt arrived, so it bounded the total from above across the whole fallback sequence. The named regression ("unretired arm overlapped a retry") is still caught, but now only byrowsAtPrompt !== 2at line 625, and only for an overlap that spawns before the wake is delivered. The two hung-successor cases keep an explicit upper bound via the post-promptstableRowscheck (lines 535-536, 2061-2062); the unretired cases have no equivalent. Mirror it: after the prompt assertions, sleep briefly and re-read armRows(), failing if the count moved past 2..opencode/plugins/fm-primary-watch-arm.js:438- Informational, not a defect in the reported failure. The new premise check snapshots onlystate/.lock, but beginArm's verdict also depends on shouldArm's inputs (state/.afk,config/x-mode.env,*.metain state) and onretryTimer. A caller arriving during the same idle turn after one of those flips inherits a stalenot-neededorretryingverdict exactly the way it used to inheritread-only. The trigger window is the isPrimaryRoot git walk (tens of ms) and the state involved changes far less often than the fleet lock, so this is much narrower than cause A, it self-heals on the next session.idle, and the reported failures were all lock-flips - the author's comment at lines 428-437 scopes the premise to the lock deliberately. Recording it so the boundary is a documented choice rather than an assumed invariant; no action needed for this change. Verified separately that the changed branch does NOT open a double-spawn window: everything fromif (child)(line 409) throughchild = armChild(line 320) runs synchronously after the sessionOwnsLock await, so a second concurrent beginArm deterministically seeschildand returnsexisting.🔧 Fix: restore fallback diagnostics and bound unretired arm counts
2 issues (1 error, 1 warning) still open:
docs/arm-readiness-determinism-proof.md:70- The required repeated-run proof still does not cover the tree it claims to prove, for the third round running. Line 5 says the numbers "are from a full run against the tree with this change's fixes applied" and line 70 says "Code under proof: this branch's commit, built directly onmainat45bd292"; the 20-idle/20-loaded phases (lines 76-77) were measured at d78a403 (git log -S"20/20 passed" -- docs/arm-readiness-determinism-proof.mdreturns only d78a403). Since that measurement, 5f56cc7 rewrote the wait structure of all four fallback cases (tests/fm-pi-watch-extension.test.sh:512-527, 615-632, 2040-2055, 2143-2160) - which the doc's own bullet at lines 44-49 calls "the other half of the cause B fix, and the one that removes the elapsed-time dependence rather than shrinking it" - and 1fce567 added a new post-promptstableRowsassertion to both unretired cases (lines 628-630, 2159-2161) that did not exist when the 40 runs were taken. 1fce567 touched only the "What this change adds" section; the Verification block is byte-identical to d78a403's. So the change's headline determinism mechanism and one entirely new assertion have zero repeated-run evidence, while the doc presents 40/40 as if they did. The intent requires "Prove the fix by running the suite repeatedly (at least 20 consecutive runs on an idle machine and at least 20 consecutive runs under substantial simulated CPU load) and report the exact pass/fail counts as counts", and "If any regression test or proof claim's own supporting evidence stops matching the implementation it describes as the implementation evolves, correct the claim rather than leaving it stale - this has been a recurring failure mode tonight and gets no exception here." Round 2 and round 3 both explicitly ordered this rerun as a final pass and it was not performed either time. The restructure looks like a strict headroom improvement (two sequential budgets instead of one), so this is an evidence gap rather than a suspected regression - but it needs the rerun plus a Verification block naming the actually-measured tree (parent commit plus what sits on top, per the round-3 instruction).docs/arm-readiness-determinism-proof.md:13- The doc's cause table assignsOpenCode watch plugin must arm only when this session owns the fleet lock(both reported occurrences) to cause A - the unconditional in-flight reuse inensureArm- but the pre-change test could not reach that race, and the base test says so itself. At 45bd292 the case ran: write foreign lock 999999,await hooks.event(event)(which firesvoid ensureArmfix(bin): refuse teardown while a task's recorded PR is still open #1, settinglaunchInFlightsynchronously), thenawait coordinator.ensureArmed(...)(feat(bin): render the captain's four-section status board from existing state #2, which joins fix(bin): refuse teardown while a task's recorded PR is still open #1). Both callers therefore evaluate the SAME lock content; the flip toprocess.pidhappens strictly after that await resolves, and by thenlaunchInFlightis already null (V8 runs fix(bin): refuse teardown while a task's recorded PR is still open #1'sawait launchreaction before feat(bin): render the captain's four-section status board from existing state #2's, so fix(bin): refuse teardown while a task's recorded PR is still open #1'sfinallyclears it first), so the third attempt evaluates fresh. The base test's own comment states this was deliberate: "Flipping the lock before that point let the next event coalesce onto the stale read-only answer and never arm at all" - i.e. the test had already been restructured to avoid cause A. The remaining timing-dependent step in that case is the 5sfor (let i = 0; i < 250 && !existsSync(FM_ARM_LOG); i += 1)poll waiting on an arm child started throughbash -lcat ~1150ms (max 1740ms) under contention - which is cause B by the doc's own definition, making it 4 assertions sharing one cause rather than "Two independent causes, two assertions each" (line 9). Note this does not undermine the fix: theensureArmrace is genuinely reachable in production via the turn-end guard'scoordinator.ensureArmedarriving while a foreign-lock attempt is pinned in itspswalk, the premise-validated coalescing is the right fix, and the rewritten test now forces that race deterministically. Only the attribution of the two reported lock-assertion failures to cause A is unsupported, and the intent requires reporting which assertions share a cause and correcting claims whose evidence does not match.🔧 Fix: correct lock-assertion cause attribution in proof doc
3 issues (1 warning, 2 infos) still open:
docs/arm-readiness-determinism-proof.md:9- The round-4 doc fix swapped one unsupported attribution for another and now contradicts itself. Line 9 states "All four reported failures are attributable to cause B" and the table row at line 14 assigns the lock assertion's two occurrences to "B - test window racing an unrelated cost"; line 26 restates it flatly ("what actually made it flaky was the same cause B confound as the other three: a 5sexistsSyncpoll waiting on an arm child started throughbash -lc"). Line 27 then says the opposite: "Why those two field failures occurred cannot be reconstructed further from the base test's mechanics." The doc's own evidence does not support the cause-B claim for this case the way it does for the other three. Traced against the base test at 45bd292 (tests/fm-pi-watch-extension.test.sh:1400-1404 at that commit), the only timing-dependent step isfor (let i = 0; i < 250 && !existsSync(FM_ARM_LOG); i += 1) await sleep(20)- a 5s budget - against the doc's own measuredbash -lcfigure of ~1150ms, max 1740ms under contention (line 35), i.e. ~2.9x headroom. Under loadsetTimeout(20)drifts long, so the loop's real wall budget grows past 5s rather than shrinking, widening that headroom further. By contrast the two Pi fallback cases had a 250ms window that the same cost plainly exceeded, which is why cause B is demonstrated there. The round-4 instruction asked only to stop claiming the base test proves cause A explains the field failures; substituting an undemonstrated cause B instead re-creates the same defect the finding was about. The intent requires "Report which of the four originally-failing assertions share one cause and which are separate" and "If any regression test or proof claim's own supporting evidence stops matching the implementation it describes ... correct the claim rather than leaving it stale." Suggested correction: mark the two lock-assertion occurrences as not determinable from the base test's mechanics (keeping the cause-B poll as a candidate rather than a conclusion), state that only the two Pi fallback assertions are demonstrably cause B, and drop the "all four" framing at line 9 and the flat assertion at line 26 so they stop contradicting line 27.tests/fm-pi-watch-extension.test.sh:1705- test_watch_arm_login_shell_default_reaches_the_arm_child is the only case in the suite that still pays the unbounded profile-sourcing cost (it runsenv -u FM_WATCH_ARM_NO_LOGIN_SHELLfor itsloginmode), and it measures that cost inside a bounded elapsed-time poll:for (let i = 0; i < 500 && !markerLogged(); i += 1) await sleep(20)at line 1705 (OpenCode branch) and line 1743 (Pi branch), i.e. a 10s bound. That is the exact confound shape - unbounded /etc/profile cost inside a bounded window - this change exists to remove, reintroduced with more headroom (10s vs the doc's measured 1740ms worst case). The header comment at lines 52-54 ("the one exception is test_watch_arm_login_shell_default_reaches_the_arm_child below, which owns no readiness window") and the proof doc's test table at line 75 both omit this. "Owns no readiness window" is defensible about the adapters' own readiness timeouts - the Pi command handler calls startArm synchronously without awaiting readiness, and the OpenCode branchvoids ensureArm so waitForArmReady's 12s timeout never retires the child - but the test still has its own 10s bound, so the statement understates what is being measured. On a host whose /etc/profile.d does anything genuinely slow, this new case is the one that can flake under the variable load the intent says the fix must survive. Either widen/justify the bound explicitly or correct the comment and doc to say the bound exists and why 10s is considered sufficient headroom.docs/arm-readiness-determinism-proof.md:81- Recorded for completeness, not as a new blocker: the Verification block's 20-idle/20-loaded counts were measured at d78a403 (git log -S"20/20 passed" -- docs/arm-readiness-determinism-proof.mdreturns only that commit), while 5f56cc7 subsequently rewrote the wait structure of all four fallback cases and 1fce567 added the post-promptstableRowsassertions to both unretired cases (tests/fm-pi-watch-extension.test.sh:628-630, 2159-2161). Line 5 ("from a full run against the tree with this change's fixes applied") and line 81 ("Code under proof: this branch's commit") therefore still name a tree that was not the one measured. The user explicitly deferred this in the round-4 instruction ("Do NOT start or continue the 20-idle+20-loaded proof rerun in this round ... the operator will run that proof separately as a dedicated final step"), so no action is requested here - it is noted only so the outstanding intent requirement ("at least 20 consecutive runs on an idle machine and at least 20 consecutive runs under substantial simulated CPU load ... exact pass/fail counts") is visible against the tree being merged.🔧 Fix: restore two-cause attribution and note login-shell residual bound
1 warning still open:
tests/fm-pi-watch-extension.test.sh:1663- The last round corrected the "owns no readiness window" claim in two of the three places it appears but left the third stating the superseded version as fact. The file header (lines 55-58) now reads "That case owns no ADAPTER readiness window (neither adapter awaits arm readiness on the path it drives), but it does own a 10s wait of its own for the arm child to record itself, so its login branch is the one place in this suite that still times an unbounded profile-sourcing cost", and docs/arm-readiness-determinism-proof.md:36-38 records the same residual. The per-test comment immediately above test_watch_arm_login_shell_default_reaches_the_arm_child still says "this case owns no readiness window, so paying the profile cost here costs the timed cases nothing" - unqualified, and 45 lines above the inline comment at line 1710 that says the opposite ("A bounded wait on an inherently unbounded cost, which this one case cannot avoid"). A reader who stops at the function's own comment concludes nothing here is timed, which is the exact claim the round-5 finding existed to remove. The intent makes this a no-exception item: "If any regression test or proof claim's own supporting evidence stops matching the implementation it describes as the implementation evolves, correct the claim rather than leaving it stale." Fix: qualify line 1663 the same way the header does - no adapter readiness window, but a 10s wait of its own on the login branch, deliberately.🔧 Fix: qualify stale readiness-window claim on login-shell test
3 issues (1 warning, 2 infos) still open:
tests/fm-pi-watch-extension.test.sh:40- The tail of the header comment block this change rewrote still states two claims the change itself invalidated. Line 39-41 reads "The budget must stay well above a loaded login-shell start; the cases are otherwise unchanged, and an arm that IS ready still settles immediately rather than waiting it out." (a) "The budget must stay well above a loaded login-shell start" is inherited from main's timeout-raise rationale and is now false for this suite: line 61 exports FM_WATCH_ARM_NO_LOGIN_SHELL=1, so every timed case spawns its arm underbash -cand the readiness budget no longer has to cover a login-shell start at all - the paragraph immediately above it (lines 43-60) says exactly that, so the block contradicts itself. (b) "the cases are otherwise unchanged" is false: this branch restructured all four cases that paragraph is about - test_pi_hung_successor_falls_back_to_typed_wake (lines 517-527), test_pi_unretired_successor_falls_back_without_retry (lines 613-621), and their two OpenCode counterparts (lines 2059-2069, 2158-2168) - replacing the single elapsed-time wait with a row-count wait plus a separate bounded prompt wait plus a post-settle stableRows re-read. The intent makes this a no-exception item ("If any regression test or proof claim's own supporting evidence stops matching the implementation it describes as the implementation evolves, correct the claim rather than leaving it stale"), and the first half of this same block was already corrected in an earlier round while this tail was left as-is. Fix: rewrite lines 39-41 to say the budget no longer has to cover a login-shell start in this suite because the opt-out removed that cost, and that the four cases now wait on the observable arm-row count rather than on elapsed time..opencode/plugins/fm-primary-watch-arm.js:443- Informational, recorded so the boundary is a documented choice.launchInFlightLockis assigned the snapshot taken at ensureArm entry, not the lock content beginArm actually evaluates - beginArm awaits isPrimaryRoot (twogit rev-parsesubprocesses) before sessionOwnsLock reads the file, so the recorded premise can be older than the evaluated one. That is conservative in one direction (a mismatch only forces an extra evaluation), but it leaves one ABA residual: if the lock reads as ours at ensureArm entry, flips foreign during the isPrimaryRoot window so beginArm computesread-only, then flips back to ours before a second caller arrives, that caller's snapshot matcheslaunchInFlightLockand it coalesces onto aread-onlyverdict that is no longer true. Reachability is far narrower than the race being fixed (it needs two lock rewrites inside a tens-of-ms window, versus the single reacquisition cause A needs) and it self-heals on the next session.idle, so this is not a defect in the reported failure. Capturing the premise inside beginArm at the point sessionOwnsLock reads it would close it, but that trades one narrow window for another (a caller arriving before the read). No action needed for this change.docs/arm-readiness-determinism-proof.md:85- Recorded for completeness only - the user explicitly deferred this in the round-4 and round-5 instructions ("Do NOT start or continue the 20-idle+20-loaded proof rerun in this round ... the operator will run that proof separately as a dedicated final step"), so no action is requested here and it is not a blocker at this phase. State of the tree: the 20-idle/20-loaded counts on lines 91-93 were measured at d78a403 (git log -S"20/20 passed" -- docs/arm-readiness-determinism-proof.mdreturns only that commit), while 5f56cc7 subsequently rewrote the wait structure of all four fallback cases and 1fce567 added the post-prompt stableRows assertions to both unretired cases. Line 5 ("from a full run against the tree with this change's fixes applied") and line 85 ("Code under proof: this branch's commit") therefore still name a tree that was not the one measured. Assertions per run (32) is still accurate - I counted 32passcalls and 32 invoked cases. Noting it only so the outstanding intent requirement ("at least 20 consecutive runs on an idle machine and at least 20 consecutive runs under substantial simulated CPU load ... exact pass/fail counts") stays visible against the tree being merged.🔧 Fix: correct stale readiness-budget claims in suite header
✅ Re-checked - no issues remain.
🔧 **Test** - 1 issue found → auto-fixed ✅
docs/arm-readiness-determinism-proof.md:29- The~1150ms (max 1740ms)loadedbash -lcfigure that three of this change's new arguments rest on understates the worst case on this host. Re-measured at the same 5x oversubscription the proof's own loaded phase uses (160 busy loops / 32 cores, loadavg ~151, 30 samples): min 1096ms, median ~1620ms, max 4246ms (two samples above 3.9s); idle matched at 131ms median. The figure itself is inherited from main (base suite line 30), but this change builds new claims on it: docs/arm-readiness-determinism-proof.md:29 argues the lock assertion's pre-change 5s budget had "ample headroom" against "at most ~1740ms" and therefore excludes cause B from that assertion - against a real 4246ms worst case that is ~18% headroom, not ample. (The cause-A attribution itself survives independently: revertingensureArmreproduced that failure deterministically.) The same understated basis backs docs/arm-readiness-determinism-proof.md:38 and the two10s bound sits well above the ~1740ms worst casecomments at tests/fm-pi-watch-extension.test.sh:1724 and :1767 - that bound did hold in all 40 runs, but with ~2.4x the stated margin consumed. Only the author can decide whether to re-measure and restate the figure or reword the headroom arguments that cite it; the intent explicitly forbids leaving a proof claim's supporting evidence stale.tests/fm-pi-watch-extension.test.shx20 consecutive on an idle host (loadavg 0.29-1.05) - 20 passed, 0 failed, 32/32 assertions each runtests/fm-pi-watch-extension.test.shx20 consecutive under 160 busy-loop processes on 32 cores (5x oversubscription, loadavg 113->163) - 20 passed, 0 failed, 32/32 assertions each runCause-A negative control: revertedensureArmto unconditional in-flight reuse (if (launchInFlight)) and reran the suite -not ok - OpenCode watch plugin must arm only when this session owns the fleet lockvia the 20s guard (the reacquired-lock arm attempt never settled)Cause-A scope control: same pre-changeensureArmwith the lock case skipped -test_opencode_watch_arm_coalesces_callers_on_an_unchanged_lockpasses, confirming it does not itself catch cause ARejected-variant control: rewroteensureArmto fully serialize callers - lock case deadlocks on the settling guard, coalescing case fails withtwo callers on an unchanged lock must share one evaluation, got 2tests/fm-operational-input.test.sh- all 8 assertions pass including the newtest_adapter_surfaces_encoder_exit_instead_of_killing_the_hostEPIPE negative control: removedchild.stdin.on("error")from.opencode/plugins/lib/fm-operational-input.js- host node process dies with unhandled EPIPE, test reportsOpenCode adapter host died (exit 1)Cause-B baseline control:git show 45bd292:tests/fm-pi-watch-extension.test.shrun x10 under 5x load (0/10 passed) and x12 under 1.5x load (3/12 passed), failing four different assertions across runsLogin-shell cost re-measurement: 20 idle and 30 loaded samples ofbash -lc truevsbash -c trueat loadavg ~151Constraint checks on the diff: noFM_*_TIMEOUT_MSvalue raised, no test invocation removed (30 assertions on base -> 32 now), no skip/flaky marker addedPost-experiment verification:git status --porcelainclean at HEADa15d993, suite green again on the restored tree🔧 Fix: correct stale login-shell contention figures in determinism proof
✅ Re-checked - no issues remain.
bash tests/fm-pi-watch-extension.test.shx20 on an idle host (load average 0.9) - 20 passed, 0 failed, 32 assertions per runbash tests/fm-pi-watch-extension.test.shx20 under 160 busy loops on 32 cores (5x oversubscription, load average 141->162) - 20 passed, 0 failedPre-change control:git show 45bd292:tests/fm-pi-watch-extension.test.shrun x4 on the same host under the same 5x load - 0 passed, 4 failed, all onPi redundant tool call must remain an ownership-based no-op with repair-only guidancebash tests/fm-operational-input.test.sh(covers the newtest_adapter_surfaces_encoder_exit_instead_of_killing_the_hostEPIPE case)Re-measuredbash -lc true/bash -c truestartup cost idle (20 samples) and at 5x oversubscription (30 samples), and checked each headroom claim the change cites against the fresh numbersNegative control: revertedensureArmto the pre-change unconditional in-flight reuse ->test_opencode_primary_watch_plugin_requires_session_lockfails withthe reacquired-lock arm attempt never settled; rantest_opencode_watch_arm_coalesces_callers_on_an_unchanged_lockalone against the same reverted code -> still passes; restored -> greenNegative control: removedchild.stdin.on("error")from.opencode/plugins/lib/fm-operational-input.js-> host node process dies with unhandled EPIPE and the test reports the host as dead; restored -> greengit status --porcelainafter all negative controls - worktree clean, no transient files left behind🔧 **Document** - 1 issue found → auto-fixed ✅
docs/arm-readiness-determinism-proof.md:96- docs/arm-readiness-determinism-proof.md's Verification table (idle 20/20, loaded 20/20, "Code under proof: this branch's commit") records a run taken at d78a403, the commit that introduced the fixes. Six later in-branch commits reworked the suite it measures - 5f56cc7 alone rewrote ~300 lines of tests/fm-pi-watch-extension.test.sh to wait on observable arm-row counts and share the gate shims, and 1fce567/22d3263/62b37d5/9c67804 changed fallback diagnostics, the login-shell wait, and the suite header. No commit records a re-run at the final tree, so the 40/40 figures no longer describe the suite as shipped (the production fixes under proof are unchanged since d78a403, so the substance is likely still true, but the counts are not evidence for this tree). Resolving it requires re-running the 20 idle + 20 loaded suite executions and re-stamping the table, which belongs to the verification phase, not this documentation phase; I deliberately did not soften or re-word the numbers because doing so without a run would substitute a guess for evidence.🔧 Fix: stamp determinism proof verification at a15d993 with provenance
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.